Skip to content

Implement (m|mun)lockall for Linux and *nix libcs #19203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

blblack
Copy link
Contributor

@blblack blblack commented Mar 6, 2024

No description provided.

lib/std/c.zig Outdated
// All the Unix-likes except darwin support the call and the flags in a
// consistent way except Linux, which has an extra flag MCL_ONFAULT and has
// non-standard values for the MCL constants on ppc, ppc64, and sparc.
pub usingnamespace switch (native_os) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#19195 (comment) applies here as well.

@blblack
Copy link
Contributor Author

blblack commented Mar 18, 2024

@Vexu - Sorry, my updated commit is failing tests on windows and others now. This is because it included a smoke test in lib/std/os/test.zig which was excluding non-supporting platforms via @hasDecl, which in turn was relying on the usingnamespace switch to only declare these interfaces on supporting platforms. Since the simple way of removing usingnamespace declared them for all targets, that doesn't work anymore (they all have the decl, and instead we get an undefined symbol at link time for the test).

I'm not yet sure how to deal with this properly, but it seems like maybe I should bring this testing topic up more-generally back over in #19214 . I suspect none of the commits there really addressed this, because all of the cases that were platform-conditional in those commits also lacked tests.

@mikdusan
Copy link
Member

mikdusan commented Mar 19, 2024

related: andrew just opened #19352

How about this pattern, makes the test gate relatively simple:

const native_os = @import("builtin").os.tag;

// suggest put this in lib/std/c.zig between
// `pub const fstat = ....` and `pub const stat = ...`
//  many conditionals there, and in alphabetic order.
pub const mlockall = switch (native_os) {
    .linux, .freebsd, .netbsd, .openbsd, .dragonfly, .solaris => private.mlockall,
    else => {},
};

// this struct already present at bottom of lib/std/c.zig
const private = struct {
    pub extern "c" fn mlockall(flags: c_int) c_int;
};

test {
    if (@TypeOf(mlockall) == void) return error.SkipZigTest;
    // use it
    _ = &mlockall;
}

@blblack
Copy link
Contributor Author

blblack commented Mar 19, 2024

That does seem like it should work. I do wonder though, in light of #19352, whether if (@TypeOf(mlockall) == void) is really any better than if (@hasDecl(mlockall)). I guess for now, it wins on at least finding a way to work without usingnamespace.

@blblack
Copy link
Contributor Author

blblack commented Mar 19, 2024

New push basically does things as @mikdusan suggested. I also used the void TypeOf check pattern to return an error.NotImplemented at the std.os layer (which I think of as the future std.posix layer, really). This seems like a reasonably-robust solution, with the caveat that the void TypeOf check still seems a little questionable in general (but less-questionable, perhaps, than other pathways attempted so far!).

@blblack
Copy link
Contributor Author

blblack commented Mar 19, 2024

Note also this should wait to be rebased+amended around whatever comes of #19354 as well.

@blblack
Copy link
Contributor Author

blblack commented Mar 20, 2024

Marking as draft for now, because I'm betting I should just wait for https://github.com/ziglang/zig/tree/linux-type-safety to land (I assume it will shortly, since this seems to be an area of focus!) and then rework the os/linux.zig bits around that as well. And honestly, I'm still unsure about what the Right Way is becoming on the platform-dependent bits.

In any case, another interim push incoming shortly that updates for latest master (incl the new std/posix.zig stuff). This one adds an @hasDecl check ahead of the @TypeOf check, because we also have to cover cases like plan9 which don't use libc and wouldn't have the decl available to check the voidness of, and makes those cases a @compileError("Unsupported OS") instead of return error.NotImplemented, as that seems more-consistent with existing practice.

I still suspect some will hate the current @hasDecl + @TypeOf approach to detect platform support at the std.posix layer for this. One alternative would be to copy the supported-systems conditional list from std/c.zig to std/posix.zig as an explicit platform check, but personally I dislike the redundancy there. Some might suggest then to kill the redundancy by removing the conditionals on the std/c.zig side and just always declaring the C interface even when it's not present, but that also seems ugly, as then users' use of the C/system interface on a non-supporting platform results in a link-time error for lack of the symbol, rather than something earlier and friendlier.

There's a lot of layers involved here, and the existing related examples in master's std still have a blend of different approaches that haven't coalesced into a single consistent design pattern that works for all cases and avoids all the obvious pitfalls (untestability, late link-time failures, copying platform-support lists around N places and letting them bitrot, etc).

This is just like std.testing.expectError, but takes a slice of
multiple errors as an "[]const anyerror" argument with the
expectation that any one of those errors should occur.
@blblack
Copy link
Contributor Author

blblack commented Mar 21, 2024

Ok, after some more thinking and conversation elsewhere, I think I've come around to a decent pattern with the latest push. This seems about as elegant as I can make things without causing other undesirable tradeoffs! There's no @hasDecl anywhere, no funky void type checks, no conditionals into private structs, etc, etc.

Basically, rather than trying to put the libc parts together in lib/std/c.zig with the strange conditional and private struct stuff, I'm just redundantly defining the interfaces down in lib/std/c/foo.zig for the supporting foos. While this is slightly-redundant, it's much simpler. It also seems more-correct from the POV of future developers who add new platforms and/or maintain existing ones, vs having them wade through the conditionals in the top-level c.zig.

At the POSIX layer, we have an explicit target native_os list that we support (which seems common and reasonable as a pattern), and we return error.NotImplemented for any other platform. This makes it testable everywhere. We can exercise the POSIX-layer code and even exercise the basic sanity of the underlying syscall where it exists.

This seems like a worthy pattern for implementing similar things (and maybe refactoring some of the existing sys/libc/posix calls towards).

@andrewrk
Copy link
Member

andrewrk commented May 9, 2024

still a draft, no update in 30+ days

@andrewrk andrewrk closed this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants